feat: parse_float_as_decimal supports scientific notation and Decimal256
#13806
Conversation
| select 123456789.0123456789012345678901234567890, | ||
| arrow_typeof(123456789.0123456789012345678901234567890) | ||
| ---- | ||
| 123456789.012345678901 Decimal256(40, 31) |
There was a problem hiding this comment.
Display is incomplete due to rounding in sqllogictest
datafusion/sql/src/expr/value.rs
Outdated
| // No decimal point, keep as is | ||
| (trimmed.len(), 0, Cow::Borrowed(trimmed)) | ||
| }; | ||
| /// Callers ensure the value is within i256 range |
There was a problem hiding this comment.
Don't have to, the function is fallible
There was a problem hiding this comment.
I reorganized it and tried to clarify it in 49a4e0a
datafusion/sql/src/expr/value.rs
Outdated
| scale as i8, | ||
| ))) | ||
| } else if precision <= DECIMAL256_MAX_PRECISION as u64 { | ||
| let val = bigint_to_i256(int_val)?; |
There was a problem hiding this comment.
either add comment like above (// Failures are unexpected here as we have already checked the precision), or handle error nicely.
| }; | ||
| /// Returns None if the value can't be converted to i256. | ||
| /// Modified from <https://github.com/apache/arrow-rs/blob/c4dbf0d8af6ca5a19b8b2ea777da3c276807fc5e/arrow-buffer/src/bigint/mod.rs#L303> | ||
| fn bigint_to_i256(v: &BigInt) -> Option<i256> { |
There was a problem hiding this comment.
please add unit tests for this function
it should be easy since both BigInt and i256 can convert to/from str
There was a problem hiding this comment.
Added, thank you @findepi
|
|
||
| let number = replaced_str.parse::<i128>().map_err(|e| { | ||
| fn parse_decimal(unsigned_number: &str, negative: bool) -> Result<Expr> { |
There was a problem hiding this comment.
Please add unit tests for this function, to exercise bound checks.
There was a problem hiding this comment.
I think the new SLT tests have covered them, but please remind me if I've overlooked anything
There was a problem hiding this comment.
they likely do, but i find it valuable to have unit tests, especially for a code which is well isolated (easy to unit tests) and has so many IFs (valuable to unit test).
There was a problem hiding this comment.
Make sense. Added in 2077749
|
shipit |
|
Thank you @findepi for the review |
Which issue does this PR close?
Prerequisite of #12817
Rationale for this change
Parse float string as
BigDecimal, then convert it toDecimal128orDecimal256.What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No breaking changes